-
Notifications
You must be signed in to change notification settings - Fork 791
[CI][L0] Modify files to enable installing L0 from .sh file #20411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: sycl
Are you sure you want to change the base?
Conversation
14d1f57 to
ea190b7
Compare
ea190b7 to
66b3302
Compare
66b3302 to
38bd413
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't fetch and build should be unconditional if commit hash is given? Now it is possible to set ZE_LOADER_LIBRARIES to /usr/lib/x86_64-linux-gnu/libze_loader.so
| string(REGEX MATCH "^[0-9a-fA-F]+$" IS_HEX "${UR_LEVEL_ZERO_LOADER_TAG}") | ||
|
|
||
| if(PkgConfig_FOUND AND NOT (TAG_LENGTH EQUAL 40 AND IS_HEX)) | ||
| pkg_check_modules(level-zero level-zero>1.24.3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably >=, as it was above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| # just try to search for the path. | ||
| if(PkgConfig_FOUND) | ||
| pkg_check_modules(level-zero level-zero>=1.24.3) | ||
| string(LENGTH "${UR_LEVEL_ZERO_LOADER_TAG}" TAG_LENGTH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why UR_LEVEL_ZERO_LOADER_TAG is checked, and not set here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaik this line of code checks the length of a given string and this length is checked in if-statement one line below. I think this is expected behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior of further if conditions may be bad because if the variable is not set, its LENGTH is 0
38bd413 to
87fa832
Compare
bcf6515 to
0f571ed
Compare
14636e7 to
70ee958
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cmake script lgtm.
|
|
||
| set(UR_LEVEL_ZERO_LOADER_REPO "https://github.com/oneapi-src/level-zero.git") | ||
| # Remember to update the pkg_check_modules minimum version above when updating the | ||
| # clone tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Make the whole comment coherent, as you mention some "minimum version above" (if fact it is below now), and in l. 20 you mention the change again.
The change allows to use a L0 commit instead of a tag which in some cases is needed by developers.